-
Notifications
You must be signed in to change notification settings - Fork 371
CIP-0168? | More BuiltinValue Functions
#1090
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
base: master
Are you sure you want to change the base?
CIP-0168? | More BuiltinValue Functions
#1090
Conversation
|
Tagging people who might be interested in weighing in: @lehins @WhatisRT @zliu41 @effectfully @MicroProofs @colll78 @ana-pantilie @Unisay @Quantumplation @rvcas |
|
@effectfully With respect to your reply here about I agree with you on However, for using |
zliu41
left a comment
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
- We'll need
mkEmptyValueas a primitive, otherwise in Plutus V4 there will be no way to construct an empty value. - What's the motivation for
negateValue? - It seems useful to generalize
policiesto return[(CurrencySymbol, [(TokenName, Integer)])]? - What's the motivation for
intersection? I think it can be done without a builtin, assuming we add all other builtins proposed, so this will need strong justification.
I had assumed this was covered by
Subtracting values.
I may have gotten ahead of myself on this one. The idea was to have a single builtin support arbitrary higher-level filtering functions like aiken's |
It's actually
This is also only the case for Plutus V1 through V3. From V4, The broader problem with For example, if we have a |
BuiltinValue FunctionsBuiltinValue Functions
rphair
left a comment
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Accepted as CIP candidate at today's CIP meeting based mainly on initial Plutus stakeholder response being queries about the specifics, rather than any claim of infeasibility or disinterest... and because support for this idea was also shown here although not included with the recent CIP-0153 update:
@fallen-icarus please update the link to the current rendered document & change the directory name to CIP-0168 🎉
Co-authored-by: Ryan <[email protected]>
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
This seems pretty reasonable to me. It would be nice to see the motivation expanded with examples.
| lookupTokens :: BuiltinCurrencySymbol -> BuiltinValue -> BuiltinValue | ||
| ``` | ||
|
|
||
| From these builtin functions, most of Aiken's stdlib `Value` functions can now make use of the |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
To be a bit annoying: it would make this proposal much more concrete to have an appendix listing all the interesting functions you want to implement with their implementations using the new primitives. I think they're presumably all quite simple, and it would make the case quite compelling.
Ideally we'd also be able to see how this improves the costing behaviour of these functions.
| -- that the result can make use of the `lookupCoin` builtin added in CIP-0153. It can always be | ||
| -- converted to a `List` for pattern-matching using the `valueData` builtin added in CIP-0153. | ||
| -- See the Rationale section for why `intersection` isn't used instead. | ||
| lookupTokens :: BuiltinCurrencySymbol -> BuiltinValue -> BuiltinValue |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I find the naming a bit confusing since it returns the same type it is given. How about restrictPolicyTo?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Perhaps it is better to make restrictPolicyTo be:
restrictPolicyTo :: [BuiltinCurrencySymbol] -> BuiltinValue -> BuiltinValueThis would support a higher-level lookupTokens as well as aiken's restricted_to (here).
| type BuiltinCurrencySymbol = BuiltinByteString | ||
|
|
||
| -- | Negate all values in a `BuiltinValue`. | ||
| negate :: BuiltinValue -> BuiltinValue |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Adding negate to union gives us a group on Value. The obvious missing thing to me is scalar multiplication (to give us a module). Does anyone need that?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I don't personally have a use for scalar multiplication (right now), but I could see it being useful to others.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Is negate just scalar multiplication with the scalar being -1? In that case I think instead of adding negate we should just add scale?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
@colll78 could this do what you need to do for #1090 (comment)?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
It could, but it would be significantly less efficient than negateValue (roughly 64% more expensive) if we are to assume there is a similar costing difference to multiplyInteger -1 n and subtractInteger 0 n
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
These two builtins only perform the respective arithmetic operations, scaleValue will have to traverse the given Value, which is going to make the discrepancy less pronounced.
There's a different point however: multiplyInteger was surprisingly tricky to cost, so that's gonna propagate into scaleValue (so much so that it may not even be feasible to implement that with the current costing machinery, not sure), while negateValue would be very straightforward. @kwxm what do you think?
If Kenneth agrees costing scaleValue is hard, then I personally don't mind adding negateValue right away, given your reasoning here, because even with faster caseList and casePair, the derivative negateValue is probably going to be at least an order of magnitude more expensive than a builtin one (not least because negateValue is guaranteed to produce a well-formed Value so we don't need to check any of the invariants (except for quantities being in their range), while a derivative implementation will have to, because we don't provide an unsafe builtin that could create a Value while avoiding the checks).
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
multiplyInteger -1 n is 64% more expensive than subtractInteger 0 n? If that's the case I'd be surprised, and it would suggest that either multiplyInteger or subtractInteger is probably not costed properly.
Regarding costing of multiplyInteger in general - since we cap the range of quantities, we should be able to consider it constant time.
Whether it's negateValue or scaleValue, it will be linear whether it's a builtin or not. Certainly the constant factors will differ greatly, but a builtin would be much more useful if it is asymptotically more expensive without it. If we are going to add one more builtin for the intra-era HF, are we absolutely sure that it should be negateValue / scaleValue?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
There's a different point however:
multiplyIntegerwas surprisingly tricky to cost
I think that actually doesn't matter. We're now restricting the Quantity type to be in the range [-2^127, 2^127-1] (although it might make sense to make the lower bound -2^127 + 1 to avoid surprises), so if were scaling the value v by n and v is nonempty and n is outside the Quantity range then the scalar multiplication is bound to fail for at least one of the tokens and we might as well fail right at the start. This means that we never need to consider scalars of more than size 2, so we can probably just ignore the size of the scalar and base the cost on the total number of tokens.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
We can also try both negateValue and scaleValue and if scaling by -1 isn't significantly more expnsive than negation (and I suspect that it won't be) then we may as well go for scaleValue. We could also implement scaleValue and check if the scalar is -1 and use negation internally in that case if it's faster, although it might be a little tricky to reflect any improvement from doing that in the costing.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Great points, thank you, I withdraw my concerns.
| intersection :: BuiltinValue -> BuiltinValue -> BuiltinValue | ||
|
|
||
| -- | Returns all policy ids found in the value. | ||
| policies :: BuiltinValue -> List [BuiltinCurrencySymbol] |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
What about getting the token names?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
That doesn't seem as useful to me since assets could have the same token name despite having different policy ids.
|
I believe I made a huge mistake not including Without Even if we need time to evaluate the demand and consider alternatives for some of the value operations proposed in this CIP, I believe |
|
@colll78 in Plutus V1 through V3 the builtin Value will be encoded as a Map in Data, so As you can see, there's a suggestion above regarding generalizing |
| intersection :: BuiltinValue -> BuiltinValue -> BuiltinValue | ||
|
|
||
| -- | Returns all policy ids found in the value. | ||
| policies :: BuiltinValue -> List [BuiltinCurrencySymbol] |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Did you mean
| policies :: BuiltinValue -> List [BuiltinCurrencySymbol] | |
| policies :: BuiltinValue -> List BuiltinCurrencySymbol |
or
| policies :: BuiltinValue -> List [BuiltinCurrencySymbol] | |
| policies :: BuiltinValue -> [BuiltinCurrencySymbol] |
?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Yes, but I'm not sure what is the right syntax for Data . I went off of CIP-0153.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
It should be BuiltinList BuiltinCurrencySymbol where type BuiltinCurrencySymbol = BuiltinData
-- | Negate the quantity of every token in a Value
negateValue :: BuiltinData -> BuiltinData
negateValue = BI.mkMap . negateOuter . BI.unsafeDataAsMap
where
negateOuter value =
BI.caseList'
BI.mkNilData
(\pair restCS ->
let newPair =
BI.casePair pair $ \(cs, tkMapData) ->
BI.mkPairData cs (negateTokens $ BI.unsafeDataAsMap tkListData)
in BI.mkCons newPair (negateOuter restCS)
)
value
negateTokens tks =
BI.caseList'
BI.mkNilData
(\tkPair restTk ->
let newTkPair =
BI.casePair tkPair $ \(tkName, amtData) ->
BI.mkPairData tkName (BI.mkI $ 0 - (BI.unsafeDataAsI amtData))
in BI.mkCons newTkPair (negateTokens restTk)
)
tksThe above blows up the budget when applied to even eight or nine values with less than ten unique tokens each. This alone makes it impossible for programmable tokens to go to production. |
|
After the HF, the terms under the |
|
Based on the discussion it seems sensible to implement |
|
I'm fine with |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
@fallen-icarus now that the plan to reconcile negateValue vs. scaleValue has been settled — which I think resolves the only pending issue for this update PR — I think this would be ready to merge unless @colll78 has anything to add or amend, pending some uncertainty about where the change is going. I'm just not making sense of the last comment:
I'm fine with
scaleValuebeing added to CIP-153. ... I'll update this CIP once CIP-153 has been updated.
Since it seems appropriate to add scaleValue to this CIP update PR, what CIP-0153 update would we be waiting for in the second sentence?
If in fact we're ready to make the necessary changes here, please confirm once that's done so we can get @colll78 @zliu41 to OK it and then we might arrange for a CIP editor review online so we don't have to wait until the next CIP meeting in the last week of November: especially if this PR has to be merged before continuing with any more updates (?).
|
This CIP isn't an update PR. It is a brand new CIP. AFAIU CIP-153 is for the builtins that will be added in the next intra-era hardfork. Meanwhile, this CIP is for the builtins that will be added after that hardfork, but for the new era. |
|
Thanks @fallen-icarus... I had a feeling I was missing something. I was a little disoriented because all CIP-0153 updates have currently been merged so I didn't know what updates you might be talking about. Therefore, as I understand it, you're already free to submit your own CIP-0153 update adding |
CIP-0153 added a new
BuiltinValuetype to make working with on-chain values more efficient. However, it added only a few builtin functions for working with this newBuiltinValuetype which limits its real world usability. Adding and maintaining builtin functions is costly, but the importance of validating values in the eUTxO model justifies a wider range of builtin functions for this purpose. With this in mind, this CIP proposes a few extra builtin functions to improve the usability of this newBuiltinValuetype while still trying to minimize the overall maintenance burden.(Rendered Version)